NIFI-16359 Cache commits per (path, branch) to remove per-process-group API load by jrebmann · Pull Request #11708 · apache/nifi

jrebmann · 2026-09-22T11:09:49Z

Summary

NIFI-16359 - Git Flow Registry Clients: Cache commits per (path, branch) to remove per-process-group API load

GitLabFlowRegistryClient against a self-hosted GitLab instance generates high load on the GitLab server as the number of versioned process groups grows. The load does not come from one expensive operation but from a per-process-group multiplier on the version-check path, compounded by a GitLab-specific full-history listing. This ticket proposes removing that multiplier at the shared base-class level so all Git-based registry clients benefit.

Environment / Observation

Self-hosted GitLab, many versioned process groups. Our infrastructure team flagged a high volume of GitLab REST API calls (repository/commits, repository/tree, repository/files) originating from NiFi. The volume scales with the number of versioned process groups, even when most of them are bound to the same flow at the same version.

Root cause

The dominant driver is a per-process-group multiplier, not the cost of a single listing:

The periodic "up to date" check and version listing run per process group. In AbstractGitFlowRegistryClient, getLatestVersion(...), getFlowVersions(...) and the latest-version comparison in getFlowContents(...) all call repositoryClient.getCommits(filePath, branch). With N process groups bound to the same flow (identical filePath+branch), each cycle issues N identical repository/commits requests. There is no deduplication across process groups.
repositoryClient is a singleton: getRepositoryClient(context) builds the client once (guarded by clientInitialized) and reuses the volatile instance. A cache placed on the base class is therefore shared across every process group using the component - i.e. the multiplier can be removed with a shared, in-memory cache.
GitLab-specific amplification: GitLabRepositoryClient.getCommits(path, branch) uses the gitlab4j List overload that delegates to Pager.all(), which pages the entire commit history for the path (the client sets per_page=100). Each of the N calls above is thus itself several page requests.
Content resolution (repository/tree, repository/files) is likewise performed per process group when materializing a version.

Relationship to NIFI-14837 / PR #10186

NIFI-14837 (fixed in 2.6.0, PR #10186) improved the GitHub client by

(a) limiting the commit listing to the first page and
(b) adding an in-memory SHA->commit-detail cache. Two points:

Those changes were applied only to GitHubRepositoryClient. GitLab has neither, and additionally pages the full history (point 3 above).
PR NIFI-14837 - Performance improvement GitHub Registry Client #10186 reduced GitHub load from O(processGroups x commits) to O(processGroups): it removed the full-history paging and the per-commit N+1 detail fetch. It did NOT remove the per-process-group multiplier - GitHubRepositoryClient.getCommits(path, branch) still issues a live getRef + first-page listing on every call, with no (path, branch) cache and no TTL. This simply was not visible at a small number of process groups. So GitHub scales linearly with process-group count as well.
The SHA->commit-detail cache is not the load-bearing part for GitLab. gitlab4j's commit listing returns fully-populated Commit objects (author, message, committed_date inline), so there is no per-commit detail call to eliminate. Porting that cache 1:1 to GitLab would add a dependency and complexity without reducing API calls.

Proposed solution

Primary, general fix - cache the commit listing at the base class. Add a short-TTL, size-bounded in-memory cache keyed by (filePath, branch) in AbstractGitFlowRegistryClient, wrapping the getCommits calls in getLatestVersion / getFlowVersions / getFlowContents. This collapses the per-process-group calls to one remote call per (flow, branch) per TTL window and benefits all Git clients (GitHub, GitLab, Bitbucket, Azure DevOps). TTL bounds the staleness for detecting a remote new version; invalidate eagerly on local writes (createContent, deleteContent) and on client (re)initialization so locally committed versions are visible immediately. TTL and size hard-coded initially; can be promoted to properties in a follow-up.
GitLab-specific - bound the commit listing to the first page (a small COMMIT_PAGE_SIZE) instead of Pager.all(), to stop full-history paging and cap payload size, mirroring the listing limit from PR NIFI-14837 - Performance improvement GitHub Registry Client #10186.
Optional follow-up - immutable content cache. For the repository/tree and repository/files load, add a cache keyed by (commitSha, path). Content at a fixed SHA is immutable, so this can be cached without a TTL.

Scope question for reviewers

The primary fix lives in the shared base class and benefits all four Git clients, so this ticket's scope effectively broadens beyond GitLab. Should NIFI-16359 own the base-class caching (with GitLab as the motivating case), or should the base-class change be split into a dedicated framework ticket that per-client tickets reference?

References
NIFI-14837, PR #10186

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000
Pull request contains commits signed with a registered key indicating Verified status

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using ./mvnw clean install -P contrib-check
- JDK 21
- JDK 25

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

pvillard31 · 2026-09-24T20:30:35Z


        // Get the current version (latest commit SHA) from the repository
-        final List<GitCommit> commits = repositoryClient.getCommits(filePath, branch);
+        final List<GitCommit> commits = getCommitsCached(filePath, branch);


Should the write conflict check call repositoryClient.getCommits() directly so a stale cache cannot reject the current version or accept an old version?

The conflict check now reads the current commit directly from the repository using getCommitsWithoutCache(...). The cached commit data is still used for read operations, but it is no longer used as the source of truth for write-conflict detection.

pvillard31 · 2026-09-24T20:30:35Z

    }

+    @Test
+    void commitsAreCachedWithinTtl() throws Exception {


Can the existing tests change the remote commit after the cache is populated and verify both fresh write conflict checks and changed data after invalidation?

A regression test was added that populates the commit cache, changes the remote commit, and verifies that a subsequent write detects the conflict using the fresh repository state. The test also performs a successful write afterward and verifies that the commit cache is invalidated and the changed commit is returned on the next read.

pvillard31 · 2026-09-24T20:30:35Z

            .required(true)
            .build();

+    public static final PropertyDescriptor COMMIT_CACHE_TTL = new PropertyDescriptor.Builder()


Should this property have a short default TTL so upgrading removes the API multiplier without extra operator configuration?

I would prefer to keep this property without a default value for backward compatibility. An unset or blank value currently disables commit caching, and introducing a default TTL would change that behavior for existing installations.

Would it be preferable to keep the current opt-in behavior, or should the cache be enabled by default with an explicit blank value remaining an opt-out?

jrebmann

This is my first pull request for nifi. I hope I've thought of everything and followed all the guidelines.

@pvillard31 Thanks for your help

jrebmann · 2026-09-25T08:42:37Z

            .required(true)
            .build();

+    public static final PropertyDescriptor COMMIT_CACHE_TTL = new PropertyDescriptor.Builder()


I would prefer to keep this property without a default value for backward compatibility. An unset or blank value currently disables commit caching, and introducing a default TTL would change that behavior for existing installations.

Would it be preferable to keep the current opt-in behavior, or should the cache be enabled by default with an explicit blank value remaining an opt-out?

jrebmann · 2026-09-25T08:43:43Z


        // Get the current version (latest commit SHA) from the repository
-        final List<GitCommit> commits = repositoryClient.getCommits(filePath, branch);
+        final List<GitCommit> commits = getCommitsCached(filePath, branch);


The conflict check now reads the current commit directly from the repository using getCommitsWithoutCache(...). The cached commit data is still used for read operations, but it is no longer used as the source of truth for write-conflict detection.

jrebmann · 2026-09-25T08:44:30Z

    }

+    @Test
+    void commitsAreCachedWithinTtl() throws Exception {


A regression test was added that populates the commit cache, changes the remote commit, and verifies that a subsequent write detects the conflict using the fresh repository state. The test also performs a successful write afterward and verifies that the commit cache is invalidated and the changed commit is returned on the next read.

pvillard31 · 2026-09-25T14:07:29Z

The latest LGTM but I'm a bit concerned by the addition of this new property to all registry clients. We always try to not expose too many things that may be super low level implementation details. While I understand the intent, I'm wondering if we should add this at this point. We could start just with the GitLab first-page limit and capability-description update. Or are you in a scenario where this caching is really needed because you have many instances of the same versioned flow?

cc @exceptionfactory for additional thoughts on this

exceptionfactory · 2026-09-25T14:31:47Z

The latest LGTM but I'm a bit concerned by the addition of this new property to all registry clients. We always try to not expose too many things that may be super low level implementation details. While I understand the intent, I'm wondering if we should add this at this point. We could start just with the GitLab first-page limit and capability-description update. Or are you in a scenario where this caching is really needed because you have many instances of the same versioned flow?

cc @exceptionfactory for additional thoughts on this

Thanks @pvillard31, I agree with your concern about the addition of a new optional property.

Although I appreciate the intent of preserving existing behavior, I think it is better to take an incremental approach, start with the GitLab page limit, and then revisit this question if it becomes a problem. In most use cases, the most recent values are the important ones, and visualizing extensive history in NiFi itself is less useful. So I also recommend removing the property for this initial change.

jrebmann · 2026-09-25T15:13:19Z

The latest LGTM but I'm a bit concerned by the addition of this new property to all registry clients. We always try to not expose too many things that may be super low level implementation details. While I understand the intent, I'm wondering if we should add this at this point. We could start just with the GitLab first-page limit and capability-description update. Or are you in a scenario where this caching is really needed because you have many instances of the same versioned flow?
cc @exceptionfactory for additional thoughts on this

Thanks @pvillard31, I agree with your concern about the addition of a new optional property.

Although I appreciate the intent of preserving existing behavior, I think it is better to take an incremental approach, start with the GitLab page limit, and then revisit this question if it becomes a problem. In most use cases, the most recent values are the important ones, and visualizing extensive history in NiFi itself is less useful. So I also recommend removing the property for this initial change.

Hi @exceptionfactory, Hi @pvillard31,

thank you for your comments. I completely understand your concerns.
But I have a few questions:

Does that mean the changes will be distributed across two releases?
If so, I'm afraid I can't expect the fixes to be available by the end of the year? Or can I?
Is it possible to merge selectively and in parts, or would I have to split it into two commits?

pvillard31 · 2026-09-25T15:44:53Z

From a code review process PoV, it should be two separate PRs. If you have a PR to limit the number of commits being retrieved, this can be merged today, no issue on that.

What I really want to understand is: in your environment, is limiting the number of commits we retrieve (we limited it to 10 in the Github client) not enough to solve your issue? I am asking because the change we made for the Github registry client made a huge difference in large scale deployments. Also, there is the option to now define the frequency at which the client is going to check the status for a given versioned process group. So the combination of both should significantly change the pressure on the git provider.

I'm trying to understand why/what is missing to also need the caching approach that you are suggesting. If this is indeed required, then I feel like we should think of a way to have it without exposing a property for it.

jrebmann · 2026-09-28T09:34:24Z

I've created a new issue NIFI-16391 and a new pull request #11727 with an isolated commit to limit the number of commits within the GitLabRepositoryClient.

If caching is still necessary (which I assume it will be), I will update this pull request.

…up API load

jrebmann force-pushed the NIFI-16359 branch 4 times, most recently from c828f8d to b76696b Compare September 24, 2026 07:22

pvillard31 reviewed Sep 24, 2026

View reviewed changes

jrebmann force-pushed the NIFI-16359 branch 2 times, most recently from 3912dbf to 08a92fb Compare September 25, 2026 08:39

jrebmann commented Sep 25, 2026

View reviewed changes

jrebmann force-pushed the NIFI-16359 branch from 08a92fb to 1a85614 Compare September 28, 2026 09:21

NIFI-16359 Cache commits per (path, branch) to remove per-process-gro…

fee29ff

…up API load

jrebmann force-pushed the NIFI-16359 branch from 1a85614 to fee29ff Compare September 30, 2026 09:29

Conversation

jrebmann commented Sep 22, 2026

Summary

Environment / Observation

Root cause

Relationship to NIFI-14837 / PR #10186

Proposed solution

Scope question for reviewers

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrebmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvillard31 commented Sep 25, 2026

Uh oh!

exceptionfactory commented Sep 25, 2026

Uh oh!

jrebmann commented Sep 25, 2026

Uh oh!

pvillard31 commented Sep 25, 2026

Uh oh!

jrebmann commented Sep 28, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants